Fix: proper FW revision offline error handling #16859
Conversation
WalkthroughThe pull request refactors error handling in multiple firmware revision check modules. In the connect package, the existing inline error checks are replaced with dedicated functions to detect network errors in both Node.js and React Native environments. A new wrapper function consolidates these checks. In the suite components and hooks, the error reporting mechanism now imports configuration for error scenarios, allowing it to conditionally bypass reporting based on the defined Assessment against linked issues
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🚀 Expo preview is ready!
|
Lemonexe
requested review from
a team,
mroz22,
szymonlesisz,
martykan and
marekrjpolak
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/connect/src/device/checkFirmwareRevision.ts (1)
9-12: Consider using constants for error names and messages.The implementation correctly identifies network errors across different environments. Consider extracting the error names and messages into constants for better maintainability.
+const NODE_JS_NETWORK_ERRORS = ['FetchError', 'AbortError'] as const; +const RN_NETWORK_ERROR_NAME = 'TypeError'; +const RN_NETWORK_ERROR_MESSAGE = 'Network request failed'; -const isNodeJSNetworkError = (e: Error) => ['FetchError', 'AbortError'].includes(e.name); +const isNodeJSNetworkError = (e: Error) => NODE_JS_NETWORK_ERRORS.includes(e.name as typeof NODE_JS_NETWORK_ERRORS[number]); -const isReactNativeNetworkError = (e: Error) => - e.name === 'TypeError' && e.message.includes('Network request failed'); +const isReactNativeNetworkError = (e: Error) => + e.name === RN_NETWORK_ERROR_NAME && e.message.includes(RN_NETWORK_ERROR_MESSAGE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/connect/src/device/checkFirmwareRevision.ts(2 hunks)packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts(2 hunks)packages/suite/src/constants/suite/firmware.ts(2 hunks)suite-native/device/src/config/firmware.ts(2 hunks)suite-native/device/src/hooks/useReportDeviceCompromised.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
- GitHub Check: run_android_e2e_tests
🔇 Additional comments (10)
suite-native/device/src/config/firmware.ts (2)
9-9: LGTM! Type definition updated to support flexible error reporting.The change to allow
shouldReportto be either true or false provides the necessary flexibility to control error reporting behavior.
19-19: LGTM! Disabled reporting for offline errors.This change aligns with the PR objectives to stop reporting
cannot-perform-check-offlineerrors to Sentry, as these are expected occurrences that don't warrant reporting.suite-native/device/src/hooks/useReportDeviceCompromised.ts (2)
9-9: LGTM! Clean import of error scenarios configuration.The import statement is well-placed and maintains code organization.
36-36: LGTM! Added conditional check for error reporting.The condition correctly uses the configuration to prevent reporting of errors marked with
shouldReport: false.packages/suite/src/constants/suite/firmware.ts (2)
14-14: LGTM! Type definition inherits base behavior.The
SoftWarningBehaviortype now correctly inherits theshouldReportproperty fromBehaviorBaseType, maintaining type safety while allowing flexible reporting behavior.
27-27: LGTM! Consistent error reporting configuration.The configuration matches the native implementation, ensuring consistent behavior across platforms.
packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts (2)
8-8: LGTM! Clean import of error scenarios configurations.The import statement efficiently combines both revision and hash check scenarios.
64-64: LGTM! Consistent error reporting control.The condition matches the native implementation, ensuring consistent behavior while maintaining the additional hash check functionality.
packages/connect/src/device/checkFirmwareRevision.ts (2)
107-111: LGTM! Error handling improvements look good.The changes correctly identify network errors and return appropriate error types, aligning with the PR objectives to improve error handling in React Native environment.
13-25: Verify test coverage for network error detection.The network error detection logic is crucial for proper error handling. Please ensure it's covered by unit tests.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for network error detection # Search for test files related to checkFirmwareRevision fd -e test.ts -e spec.ts checkFirmwareRevision # Search for test cases covering network errors rg -A 5 -B 5 "isNetworkError|isNodeJSNetworkError|isReactNativeNetworkError" --type tsLength of output: 3655
🏁 Script executed:
#!/bin/bash # Output the contents of the test file for checkFirmwareRevision to verify network error tests cat packages/connect/src/device/__tests__/checkFirmwareRevision.test.tsLength of output: 5500
Test coverage for network error detection verified.
- The unit tests in
packages/connect/src/device/__tests__/checkFirmwareRevision.test.tsinclude scenarios that throw aFetchErrorto simulate network errors.- The test verifies that when a network error is thrown, the function returns the
'cannot-perform-check-offline'error, confirming that the network error detection logic is correctly covered.
Lemonexe
force-pushed
the
fix/native-offline-error-in-connect
branch
from
fa3f73e to
1ba6a1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/connect/src/device/checkFirmwareRevision.ts (1)
9-11: LGTM! Consider extracting the error message check to a constant.The network error detection functions are well-implemented for both Node.js and React Native environments.
Consider extracting the error message to a constant for better maintainability:
+const REACT_NATIVE_NETWORK_ERROR_MESSAGE = 'Network request failed'; const isReactNativeNetworkError = (e: Error) => - e.name === 'TypeError' && e.message.includes('Network request failed'); + e.name === 'TypeError' && e.message.includes(REACT_NATIVE_NETWORK_ERROR_MESSAGE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/connect/src/device/checkFirmwareRevision.ts(2 hunks)packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts(2 hunks)packages/suite/src/constants/suite/firmware.ts(2 hunks)suite-native/device/src/config/firmware.ts(2 hunks)suite-native/device/src/hooks/useReportDeviceCompromised.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- suite-native/device/src/hooks/useReportDeviceCompromised.ts
- packages/suite/src/constants/suite/firmware.ts
- suite-native/device/src/config/firmware.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts (2)
8-8: LGTM!The addition of
revisionCheckErrorScenariosimport aligns with the PR objectives to improve error handling.
63-63: LGTM! Verify error reporting consistency.The addition of the
shouldReportcheck aligns with the PR objectives and follows the same pattern asuseReportHashCheck.Run the following script to verify consistent error reporting patterns:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that error reporting patterns are consistent across the codebase. # Test: Search for similar error reporting patterns. Expect: Consistent usage of shouldReport. rg -A 5 $'shouldReport.*reportCheckFail'Length of output: 41
I'll generate an additional script to inspect the error reporting context in both files. This script will print a snippet from
useReportDeviceCompromised.tsaround the check and try to locate and show similar context fromuseReportHashCheck.tsto verify that the error reporting pattern is consistent.
🏁 Script executed:
#!/bin/bash # Print a snippet from useReportDeviceCompromised.ts to inspect the error reporting check. echo "=== useReportDeviceCompromised.ts (lines 55-75) ===" sed -n '55,75p' packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts echo "" echo "=== Searching for useReportHashCheck.ts and printing its context for 'shouldReport' ===" # Locate the file(s) containing 'useReportHashCheck.ts' fd "useReportHashCheck.ts" . # For every found file, print a snippet around the occurrence of 'shouldReport' for file in $(fd "useReportHashCheck.ts" .); do echo "" echo "Snippet from $file:" rg -n -C 5 "shouldReport" "$file" doneLength of output: 1240
I'll generate another script to print the complete context of the
useReportHashCheckfunction from the file so we can verify that the error reporting pattern (and its conditional checks) is consistent with the revision check logic.
🏁 Script executed:
#!/bin/bash # Print the complete context of the useReportHashCheck function for further inspection. sed -n '/const useReportHashCheck =/,$p' packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.tsLength of output: 1733
Error reporting patterns are consistent across hooks.
BothuseReportRevisionCheckanduseReportHashCheckuse ashouldReportcheck to conditionally trigger error reporting, ensuring that errors are only reported when appropriate. The implementation inuseReportRevisionCheckmirrors the pattern seen inuseReportHashCheck, providing consistent behavior in handling error scenarios.packages/connect/src/device/checkFirmwareRevision.ts (2)
19-23: LGTM! Well-documented and type-safe implementation.The
isNetworkErrorfunction effectively combines checks for both environments with proper type safety.
105-108: LGTM! Improved error handling precision.The error handling now correctly identifies network errors across different environments, which directly addresses the PR objectives.
Description
other-error)cannot-perform-check-offlineto Sentry (does not make sense to report such a thing!)❗ root cause issue still stands - why connect does not load the releases.json and identifiy the revision there? But given the observations, it's now not necessarily mobile only! The problem could very well be on desktop too, but invisible, because on desktop, fetch error will be correctly identified as offline error, and that's not seen in Sentry. Need to investigate further..
🤔 💭 why are there offline errors on mobile, but never on desktop? Idk! Maybe because different Sentry SDK? In web/desktop SDK, it immediately sends an event to ingest, but you're offline so it never arrives. I guess mobile sends it later, when you are connected again.
Related Issue
Resolve #16725